Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

use external package to wrap React imports to circumvent non-existing functions in RSC #11175

Merged
merged 12 commits into from
Nov 8, 2023
Merged

use external package to wrap React imports to circumvent non-existing functions in RSC #11175

merged 12 commits into from
Nov 8, 2023

Conversation

phryneas
Copy link
Member

@phryneas phryneas commented Aug 28, 2023 •
edited
Loading

This would be one possible solution for the problem report we got in #11167

I really hope we'll get another solution than this, but I'll already open the PR to see if our package CI tests work with this.

Review Note:

Please also review https://github.com/phryneas/rehackt

  • In that context: See https://github.com/phryneas/rehackt/blob/main/index.js - we could wrap that require call there in a try..catch block, which would allow our main entry point to be imported even if our consumers wouldn't have react installed.
  • But we would need to export something anyways, and if that should contain a meaningful way of throwing an error message, that would increase bundle size again.

(also, release-3.9 needs to be brought up to speed with main, right now it's at the last release we had)

Checklist:

  • If this PR contains changes to the library itself (not necessary for e.g. docs updates), please include a changeset (see CONTRIBUTING.md)
  • If this PR is a new feature, please reference an issue where a consensus about the design was reached (not necessary for small changes)
  • Make sure all of the significant new logic is covered by tests

@changeset-bot
Copy link

changeset-bot bot commented Aug 28, 2023 •
edited
Loading

🦋 Changeset detected

Latest commit: e8fddb7

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@apollo/client Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@github-actions
Copy link
Contributor

github-actions bot commented Aug 28, 2023 •
edited
Loading

size-limit report 📦

Path Size
dist/apollo-client.min.cjs 37.21 KB (+0.08% 🔺)
import { ApolloClient, InMemoryCache, HttpLink } from "dist/main.cjs" 43.67 KB (+0.02% 🔺)
import { ApolloClient, InMemoryCache, HttpLink } from "dist/main.cjs" (production) 42.16 KB (+0.06% 🔺)
import { ApolloClient, InMemoryCache, HttpLink } from "dist/index.js" 32.71 KB (0%)
import { ApolloClient, InMemoryCache, HttpLink } from "dist/index.js" (production) 31.38 KB (0%)
import { ApolloProvider } from "dist/react/index.js" 1.28 KB (+4.14% 🔺)
import { ApolloProvider } from "dist/react/index.js" (production) 1.26 KB (+4.28% 🔺)
import { useQuery } from "dist/react/index.js" 4.34 KB (+1.17% 🔺)
import { useQuery } from "dist/react/index.js" (production) 4.16 KB (+1.19% 🔺)
import { useLazyQuery } from "dist/react/index.js" 4.65 KB (+1.19% 🔺)
import { useLazyQuery } from "dist/react/index.js" (production) 4.47 KB (+1.22% 🔺)
import { useMutation } from "dist/react/index.js" 2.6 KB (+2.08% 🔺)
import { useMutation } from "dist/react/index.js" (production) 2.58 KB (+2.13% 🔺)
import { useSubscription } from "dist/react/index.js" 2.29 KB (+2.31% 🔺)
import { useSubscription } from "dist/react/index.js" (production) 2.25 KB (+2.31% 🔺)
import { useSuspenseQuery } from "dist/react/index.js" 4.33 KB (+1.05% 🔺)
import { useSuspenseQuery } from "dist/react/index.js" (production) 3.79 KB (+1.58% 🔺)
import { useBackgroundQuery } from "dist/react/index.js" 3.82 KB (+1.48% 🔺)
import { useBackgroundQuery } from "dist/react/index.js" (production) 3.27 KB (+1.86% 🔺)
import { useReadQuery } from "dist/react/index.js" 3.05 KB (+1.73% 🔺)
import { useReadQuery } from "dist/react/index.js" (production) 3 KB (+1.76% 🔺)
import { useFragment } from "dist/react/index.js" 2.15 KB (+2.47% 🔺)
import { useFragment } from "dist/react/index.js" (production) 2.1 KB (+2.48% 🔺)

@phryneas
Copy link
Member Author

/release:pr

@github-actions
Copy link
Contributor

A new release has been made for this PR. You can install it with npm i @apollo/[email protected].

This was referenced Aug 28, 2023
Closed
Merged
@phryneas phryneas changed the base branch from main to release-3.9 October 16, 2023 08:15
@phryneas phryneas marked this pull request as ready for review October 16, 2023 09:08
@phryneas phryneas requested a review from a team as a code owner October 16, 2023 09:08
@phryneas phryneas self-assigned this Oct 16, 2023
jerelmiller
jerelmiller approved these changes Nov 7, 2023
View reviewed changes
Copy link
Member

@jerelmiller jerelmiller left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seeing as you've followed through all the various threads to get to this conclusion, I see no reason we shouldn't go with it. Thanks for all the diligent work you've done to find an optimal solution given where we are with the library today!

src/react/hooks/internal/useDeepMemo.ts Outdated Show resolved Hide resolved
alessbell
alessbell approved these changes Nov 7, 2023
View reviewed changes
Copy link
Contributor

@alessbell alessbell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for all your work on moving this forward!

@phryneas phryneas merged commit d6d1491 into release-3.9 Nov 8, 2023
6 checks passed
@phryneas phryneas deleted the pr/rehackt branch November 8, 2023 11:51
@github-actions github-actions bot mentioned this pull request Nov 8, 2023
Merged
@phryneas phryneas mentioned this pull request Nov 8, 2023
Open
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 9, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@alessbell alessbell alessbell approved these changes

@jerelmiller jerelmiller jerelmiller approved these changes

Assignees

@phryneas phryneas

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@phryneas @jerelmiller @alessbell